Skip to content

Comments

WA-NEW-003: Fix DataFile password randomization tests to be deterministic#647

Open
kitcommerce wants to merge 1 commit intonextfrom
wa-new-003-datafile-deterministic
Open

WA-NEW-003: Fix DataFile password randomization tests to be deterministic#647
kitcommerce wants to merge 1 commit intonextfrom
wa-new-003-datafile-deterministic

Conversation

@kitcommerce
Copy link

Closes #643.\n\nExtracted from stacked PRs #630/#631 to make merge order linear.

@kitcommerce
Copy link
Author

Dispatcher Build Gate Summary (local)

  • rubocop (diff-only): PASS (0 offenses)
  • brakeman: skipped (per repo build gate config; disabled)
  • tests (affected engines): PASS
    • core: PASS

Note: test output still prints BSON Symbol deprecation warning (expected to be addressed by WA-NEW-010 / PR #635).

@kitcommerce kitcommerce added review:architecture-pending Architecture review in progress review:security-pending Security review in progress review:test-quality-pending Test quality review in progress review:architecture-done Architecture review complete review:test-quality-done Test quality review complete and removed review:architecture-pending Architecture review in progress review:test-quality-pending Test quality review in progress labels Feb 21, 2026
@kitcommerce
Copy link
Author

Architecture Review

Verdict: PASS

Test-only change stubbing SecureRandom.hex for determinism; no architectural boundary/dependency concerns introduced.

@kitcommerce
Copy link
Author

Test Quality Review

Verdict: PASS_WITH_NOTES

Good improvement: stubbing SecureRandom.hex makes the test deterministic and adds a positive assertion that will fail if randomization breaks.

Note (low)

  • core/test/models/workarea/data_file/csv_test.rb:374 — The test asserts the exact randomized password string including a hardcoded suffix (_aA1), which couples the test to password-format/complexity implementation details.
    • Suggestion: keep stubbing randomness, but reduce coupling by asserting only the guarantees we care about (password is set, authenticates with the deterministic value from the seam you control, not equal to prior password, meets validity/length), rather than encoding a specific suffix/format.

@kitcommerce kitcommerce added review:security-done Security review complete and removed review:security-pending Security review in progress labels Feb 21, 2026
@kitcommerce
Copy link
Author

Security Review

Verdict: PASS

Test-only determinism change (stubbing SecureRandom in tests). No production security behavior changes.

@kitcommerce kitcommerce added the merge:ready All conditions met, eligible for merge label Feb 21, 2026
@kitcommerce
Copy link
Author

✅ Review summary

Wave 1 reviewers are all PASS or PASS_WITH_NOTES.

  • Architecture: PASS
  • Security: PASS
  • Test quality: PASS_WITH_NOTES (LOW)

Labeled merge:ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate:build-passed Build gate passed merge:ready All conditions met, eligible for merge review:architecture-done Architecture review complete review:security-done Security review complete review:test-quality-done Test quality review complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant